Skip to content
This repository has been archived by the owner on Jan 9, 2025. It is now read-only.

Reanimate travis build #195

Closed
wants to merge 1 commit into from
Closed

Conversation

aik099
Copy link
Member

@aik099 aik099 commented Mar 12, 2020

No description provided.

@aik099
Copy link
Member Author

aik099 commented Mar 12, 2020

@stof , Travis CI isn't picking this PR.

@stof stof closed this Mar 12, 2020
@stof stof reopened this Mar 12, 2020
@stof
Copy link
Member

stof commented Mar 12, 2020

According to https://travis-ci.org/github/minkphp/MinkZombieDriver/requests, it could not parse the .travis.yml file. You probably have an issue in it.

.travis.yml Outdated
@@ -19,6 +17,12 @@ env:

matrix:
include:
- php: 5.4
dist: trusty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this indentation is probably the error

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Thank you.

Fixed now. Will see what actual build will show.

@aik099
Copy link
Member Author

aik099 commented Mar 12, 2020

@stof , as you can see from build logs older Zombie versions (even with older NodeJS versions) can't install properly. Should we really support them?

Maybe we'll just support latest Zombie version (6.x detected automatically) with latest NodeJS version from Travis + Zombie 5.x version with NodeJS 6.x?

@stof
Copy link
Member

stof commented Mar 12, 2020

I'm all for dropping old Zombie versions.

Ideally, I'd like a way to specify the zombie dependency automatically, so that we would have control over it, but that would require that the JS part of our code is managed by a JS package manager (yarn/npm/...), which would be a big change

src/ZombieDriver.php Outdated Show resolved Hide resolved
@aik099
Copy link
Member Author

aik099 commented Mar 12, 2020

Now build is working (at least not dying) and on every PHP version the PHPUnit tests that fail are failing the same. Though I have no idea why they are failing. I've kept Zombie 4.0 installed on Node 5.4.1 as a reference to show, that it was working before.

@stof , any ideas? You can safely change PR code directly too.

Removing usage of ProcessBuilder had a side effect, that older Process class used on PHP 5.3 and PHP 5.4 doesn't like arguments as array and expects them as an escaped string already.

Not sure if we should try fixing it or drop PHP 5.3 and PHP 5.4 support 😄

@stof
Copy link
Member

stof commented Mar 13, 2020

Well, ideally, we should first do a release with the existing state before dropping old PHP versions. But you can detect support for arrays in the Process class by checking for existence of Process::fromShellCommandLine method (or something like that, I'm not sure about the exact name).

@stof
Copy link
Member

stof commented Mar 13, 2020

For the failing test for files, we probably need to use browser.attach rather than browser.fill for file inputs.

@aik099
Copy link
Member Author

aik099 commented Mar 13, 2020

Well, ideally, we should first do a release with the existing state before dropping old PHP versions. But you can detect support for arrays in the Process class by checking for existence of Process::fromShellCommandLine method (or something like that, I'm not sure about the exact name).

The method doesn't exist anymore. I'll check for Process::escapeArgument method, that indicates support for argument array conversion ability to a command line.

@aik099
Copy link
Member Author

aik099 commented Mar 13, 2020

For the failing test for files, we probably need to use browser.attach rather than browser.fill for file inputs.

I've tried that. The \Behat\Mink\Tests\Driver\Js\ChangeEventTest::testSetValueChangeEvent is structured the way, that we first reset input value and then set new value. That approach doesn't work for upload fields due assaf/zombie#1203, which I've just reported.

@aik099
Copy link
Member Author

aik099 commented Mar 14, 2020

The basic auth related tests fail because somehow on Travis CI PHP auth headers didn't reach PHP. Maybe due fact, that PHP_SAPI is CGI/FastCGI.

The \Behat\Mink\Tests\Driver\Js\ChangeEventTest::testSetValueChangeEvent tests fail, because new Zombie version fires 2 change events for 1 change for select/radio/checkbox elements. Reported as assaf/zombie#1204.

Locally fixed \Behat\Mink\Tests\Driver\Js\EventsTest::testKeyboardEvents. The issue was with usage of deprecated (probably Zombie already dropped it) usage of e.keyCode event property. The correct way is to use e.charCode property of event.

@aik099
Copy link
Member Author

aik099 commented Mar 21, 2020

If using built-in PHP web server of PHP 7.4, then Behat\Mink\Tests\Driver\Basic\CookieTest::testCookie also fails with this message:

Failed asserting that 'Basic Form Previous cookie: client+cookie+set' contains "Previous cookie: client cookie set".

@stof , I remember seeing some change where urlencode (or decode) was replaced by rawurlencode (or decode), but I don't remember where so that I can replicate that in this driver.

@aik099
Copy link
Member Author

aik099 commented Mar 22, 2020

Strange, but \Behat\Mink\Tests\Driver\Js\EventsTest::testKeyboardEvents fix haven't worked on Travis CI, but worked locally.

1. don't use "ProcessBuilder" due deprecation
2. fixed "getValue" result for an empty select
3. support "setValue" for upload fields
4. fixed keyboard event emulation
5. actualized used PHP versions
@aik099 aik099 force-pushed the reanimate-travis-build branch from c4634de to c3cb2cb Compare March 22, 2020 16:36
@aik099
Copy link
Member Author

aik099 commented Mar 22, 2020

Out of curiosity I've also tested Zombie 5.x on Travis CI and all test did pass, except one that calls setValue on upload field twice, that fails due the same reason as for Zombie 6.

@stof
Copy link
Member

stof commented Oct 11, 2021

Closing in favor of #196

@stof stof closed this Oct 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants